@NickGerleman I would be extremely grateful if you helped me with reviewing this or finding somebody to review this 😊

Base commit: 3d09b6f
Branch: main

@mdvacca would you be able to take a look at this? It relates to removing duplication for Spannable generation.

IDK what our unit test coverage looks like here.

@NickGerleman Thanks!

@mdvacca I would be grateful if you took a look at this! 🫶

Unfortunately, if I were to answer the question, it would be "not so good"; I assume that if such unit tests existed, they would be located in react-native/packages/react-native/ReactAndroid/src/androidTest, and I don't think anything there is focused on testing Spannable.

@NickGerleman @mdvacca

I can keep bumping. Let me know if there's anything I could do to move this forward

I've been checking with @mdvacca offline. I think he was sick last week, but mentioned he was planning to take a look and give some feedback. Thank you for keeping us honest here.

@mdvacca It would be awesome if you could give it a quick look before the end of the week. I could start working on eventual changes before the next round.

Hi @cubuspl42, thanks for the PR and apologies for the slow response.
I will analyze in details and provide feedback tomorrow!

@mdvacca

I pushed a new revision, addressing most comments. What is left:

How about refactoring this code to use a builder pattern?

Most usages of the builder pattern I've encountered were a substitute for the data class Kotlin copy() method; this isn't exactly a case like that.

Would you provide a few lines of code that show how things would look after the refactor so I'm sure we're on the same page?

Can you add a new feature flag in ReactFeatureFlags and refactor your code to be able to A/B this refactor?

This is going to be tricky, but it is probably possible.

From the operational perspective, you're talking about A/B tests that Meta could run on the Facebook app and/or other Meta apps, do I get this right?

@mdvacca Let me know what you think of the Kotlin "port" of the new bits. For example, I used a named object for utils, instead of exposing public functions, as for now some of the names were quite generic, like addText. This might change once we close the other topic about the builder pattern.

@mdvacca While I'm trying to take a humorous approach, this is a serious, externally sponsored effort, and we'll need multiple PRs to implement the desired functionality. I beg for some feedback loop fluency here.

I'll start figuring out the code for the feature flags, but feedback on rewriting everything to Kotlin will also be extremely helpful.

Please let me know if I, or Expensify, can do anything to improve the feedback loop situation.

@cubuspl42, will take a look at it today, thank you!

@mdvacca Thank you!

@cubuspl42

From the operational perspective, you're talking about A/B tests that Meta could run on the Facebook app and/or other Meta apps, do I get this right?

Correct, technically this would mean to add a new static variable in ReactFeatureFlags and then refactor the PR to be able to execute "old Code" or "new code" based on the value of the feature flag.

https://github.com/facebook/react-native/blob/97647d11841c9c97632617fe770439e3acb689ea/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/config/ReactFeatureFlags.java

We will take care of running the experiment at Meta and analyzing result of it.

I will share some detail of the code in a bit

Thanks for moving the new classes to internal and migrating them to Kotlin!

Would you provide a few lines of code that show how things would look after the refactor so I'm sure we're on the same page?

I did a further analysis and I think that using a builder pattern will require a much bigger refactor, I think we could leave it like this for now.

Although it will important to add a FeatureFlag as I mentioned in my comment above.

Also, let's document all new public classes, interfaces and method using KDoc

@cortinico, can you also take a quick look at this PR? in particular the Kotlin side

Thanks!

Also, let's document all new public classes, interfaces and methods using KDoc

If I'm not mistaken, I added KDocs to all new classes. Are you sure about the methods? Should I add comment docs for all Java getters, Kotlin properties, the new addFoo helpers, etc.?

It would be nice, we are also exploring doing a V7 for lottie to address this and the new install_module script on iOS (since it only supports RN 70 and above). But it would be nice to not have to worry about this too much 🙏🏻

@Kudo

i would just patch lottie-react-native for replacing UNSET to -1

I'm not an authoritative source here, but I'm 90% sure this is now considered a public API:

I'd use this on your place as a fix for the specific problem you encountered.

@cubuspl42 sounds good! i'll update the patch accordingly. thanks for the great hint.

For future reference, there's one other local definition of UNSET:

If it ever needs extracting, we could name it ReactConstants.UNSET_LONG_MIN. However, it might never be necessary.

I don't think we have any cases of UNSET = -1L. If we need it, we could name it ReactConstants.UNSET_LONG.

I think we don't need more future predicting here.

@NickGerleman Kind bump on the experiment info

What's the ETA for starting and finishing it?

@NickGerleman Kind bump on the experiment info

What's the ETA for starting and finishing it?

Because of the nature of needing to incrementally roll out to production audience, and get feedback, it will be a while before we are able to remove the forking (potentially a month or more). But it should be possible to continue building on top of it in the meantime.

@NickGerleman Thanks!

As I understand it, it's already live, and the finishing ETA is roughly 1-3 months. I don't mean to push it, I just want to establish a timeline.

Hey @cubuspl42, while rolling this out to our users, we discovered it causes a performance regression, in the time to render content, for a couple of our scenarios. More concretely, a number of users in the millions would go from "responsive" to "unresponsive" on those surfaces.

I have deallocated the experiment due to performance impact. If we wanted to try shipping this again, I think we'd need to understand where this issue presents in real apps, and come up with a fix.

Thank you for sharing these results! I must admit that they surprised me, but prove you had a good intuition to set up an experiment. Did the mentioned user group share a common configuration of React Native? Was it Fabric/bridgeless?

The surfaces were Fabric + Bridgeless. One involved a scrollable grid of components with text. One was more static. Both in an app with very large usage.

A lot of real-world Android devices are low/middle-end, which means sometimes a very minor change on higher end device, can push a surprisingly large number of real-world devices from one responsiveness threshold to another.

It was brought to my attention that someone from Meta (I'm not sure whether it was you personally) contacted Expensify, informing that Meta will no longer accept big PRs related to Text in the foreseeable future.

1. Would you like me to submit a follow-up that removes the "unified" path, or would you prefer to leave it there, so someone from Meta can investigate this topic in the future, when this will be more aligned with Meta's roadmap and priorities?

2. Other folks from Expensify are wondering whether adding test suites of some kind could change Meta's position on this matter. If you know that the answer is "definitely no," I could forward this information, and it would speed up the pivoting process on our side (possibly by months).

Thank you for your help so far; I'm sorry it ended this way.

Hey there. I think @TheSavior chatted with folks at Expensify wrt Text changes.

Our own problem right now, is that certain classes of contributions are hard to land, from outside of Meta. In these cases, where we have encouraged contribution, we've often created a bad experience for change authors (hard/slow to merge, hard to debug regressions effecting product code), coupled with at times a spikey workload for engineers at Meta, who usually don't have time budgeted ahead of time for larger changes outside their current area of focus.

I don't know that we ever explicitly chatted about what to do with in-flight changes. If there is a scoped, logical wrap-up, to changes already in flight, I think we could take a look at those. But if we think this is more likely to become dead code, I do really appreciate the offer to help clean things up.

I really appreciate all the time you spent here, working to make RN better. I am sorry, we couldn't offer a better experience here.

I do think it is a reasonable question of whether there is something that could be contributed around testing to increase our confidence in merging changes to Text. @NickGerleman do you have any thoughts there?

Meta has collectively spent some tens of thousand of hours on tooling and infra, but has usually focused on using them for Meta products, or the Meta monorepo. That puts us into a complicated state, where we do use this tooling, as a pragmatic way to ensure RN quality (for our apps, and OSS). But those tools don't come free to OSS.

Without commenting about what can be contributed vs not, there are a few classes of tooling that come to mind, that we use to ensure quality:

Fast and stable screenshot comparison testing

We get feedback on whether a change visually effects both our real-world surfaces, and fixtures we created. E.g. when I was working on the Samsung TextInput SEV a while back, I set up infra to visually compare a lot of interesting formatting span/nesting scenarios, for both Fabric and Paper (and they have since caught real regressions).

There have been some efforts to stand up OSS E2E infra before, but they have run into stability issues, and never got around to a lot of what is really useful (e.g. screenshot comparison). If we had reliable, simple infra, to make it easy for contributors to add image baselines, that would give us a lot more confidence for some changes.

Experimentation infra

We often test by observing differences in behavior/performance when a change is enabled vs not. This is inherently something that has to come from a collection of product code (e.g. when I was at another company, we did the same thing). That looks like, enabling a change for a large fleet of users, across surfaces, and having good enough metrics to understand if we see e.g. more rage shakes, crashes, perf changes, impression changes, etc.

Perf infra

I know things like the Java sampling profiler work pretty out of the box in OSS (and I think, that is probably the best place to start looking for diagnosing a perf issue for e.g. a change like this). But there is often bespoke infa Meta has to make this sort of thing easier and faster. E.g. If I see that a change makes a surface slower to start, I can go look at how much time is spent, via a sampling profiler, that ran during the duration, collecting information across real production users. Or, tooling for e.g. trace markers, which I'm not sure are wired to sinks in most OSS builds (I remember when I was at MSFT, we did wire them to Windows ETW).

To clarify though, I don't think better tooling is a magic bullet to ensure smooth contributions, though for many cases, I think it does help build confidence. I think another component, is aligning changes which will require a lot of interaction from engineers at Meta, to work the engineer is chartered to. That helps make it a lot easier to prioritize the work, and for someone to represent it or deal with any fallout when shipping it. I can say personally, at any given moment, there will be something falling off my plate that someone has asked me to do, just by the nature of the scale factor of the relatively small number of folks working on RN, compared to the very large userbase. So, it's easier to keep things above that line, when we can very easily justify the work.

These are extremely helpful thoughts; this is a communication breakthrough from my perspective.

I hope I'm not stepping out of the line here, but this has been bothering me:

Would it be possible for you to talk with your superiors and discuss Meta's priorities in the context of React Native within its Open Source umbrella project? Would be possible to establish a budget for this "communitization" effort of React Native? A few talented engineers from the Expensify project (and the supporting companies, like Software Mansion) would love and be able to help with this, but I'm not going to lie; these are very difficult technical tasks requiring expert knowledge, experience and time.

The budget established for implementing spans (eventually, inline code blocks) in Text wouldn't cover implementing the testing infrastructure prototype. Also, it may not directly fall into a category of efforts that Expensify could manage to sponsor in general. The gains from these efforts would benefit the whole community, though.

The facebook/react-native Android Test Suite was disabled for components that rely on Yoga for layout (commit 709a441).
It could be solved using the mock YogaConfigFactory to run those tests with roboeletric.

I don't believe Yoga layout is essential to run tests for Text and TextInput.

• We could just mock the height/width of the EditText or TextView. No need to compute height/width with Yoga for the test case.
• The Text/TextInput spans do not use Yoga Layout. It is an Android API named Spans. We don't need to have Yoga to verify that they work correctly.

More info in PR #35949 and more useful code pointers in comment #35130 (comment).